fix(vis-server): add Host-header guard to block DNS rebinding#67
fix(vis-server): add Host-header guard to block DNS rebinding#67aaronjmars wants to merge 1 commit into
Conversation
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17717696fb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const colon = trimmed.indexOf(':'); | ||
| return (colon >= 0 ? trimmed.slice(0, colon) : trimmed).toLowerCase(); |
There was a problem hiding this comment.
hostnameFromAuthority splits non-bracketed input at the first colon, so values like VIS_HOST=2001:db8::10 normalize to 2001 instead of the full address. The guard then rejects valid requests whose Host is [2001:db8::10] with 403 FORBIDDEN_HOST, even though they match the configured bind host. This introduces a functional regression for IPv6 bindings unless operators discover and use bracketed forms everywhere.
Useful? React with 👍 / 👎.
| ): boolean { | ||
| if (isLoopbackHost(hostname)) return true; | ||
| if (allowedHosts.has(hostname)) return true; | ||
| if (normalizedBindHost.length > 0 && hostname === normalizedBindHost) return true; |
There was a problem hiding this comment.
The host check only allows an exact match with normalizedBindHost, which breaks wildcard binds (VIS_HOST=0.0.0.0 or VIS_HOST=::). In those modes, real clients connect using a concrete LAN/public hostname or IP, not 0.0.0.0/::, so requests are rejected as forbidden unless every reachable host is manually duplicated in VIS_ALLOWED_HOSTS. This regresses the existing non-loopback access flow that was previously enabled by setting a bind host plus auth token.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed at the wiring level rather than here: as of d1832a7 the guard is installed only when no VIS_AUTH_TOKEN is set (see createApp), and a non-loopback/wildcard bind already requires a token (config.ts throws otherwise). So for VIS_HOST=0.0.0.0/:: the guard isn't mounted at all — the token is the access control and concrete LAN clients are served normally. test/host-guard.test.ts verifies this (token + 0.0.0.0 bind + a 192.168.x client is not 403). The exact-match line flagged here only runs in no-token loopback mode, where the bind host is loopback. Additionally isWildcardHost already drops a wildcard bindHost as a match target, so even a direct call wouldn't rely on 0.0.0.0 matching.
|
@codex review Thanks for the careful pass — both P1s were real functional regressions in the host guard. Addressed in 1. Unbracketed IPv6 authorities ( 2. Wildcard binds ( Regression coverage added: bare-IPv6 parsing, the bracketed-Host round trip, and the wildcard-requires-allowlist contract (including that the On the changeset-bot warning: no changeset is included on purpose — the only touched package is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b741fce752
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| normalizedBindHost: string, | ||
| allowedHosts: ReadonlySet<string>, | ||
| ): boolean { | ||
| if (isLoopbackHost(hostname)) return true; |
There was a problem hiding this comment.
127.
hostnameAllowed trusts isLoopbackHost(hostname) for untrusted request hostnames, but that helper treats any string beginning with 127. as loopback. As a result, attacker-controlled domains like 127.0.0.1.nip.io or 127.evil.com are accepted as loopback and bypass the new DNS-rebinding guard, allowing the exact cross-origin access this change is meant to block when the server runs in default no-token loopback mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in d1832a7 — isLoopbackHost no longer uses startsWith('127.'). It's now a strict 127.0.0.0/8 dotted-quad match (LOOPBACK_IPV4 in config.ts), so 127.0.0.1.nip.io and 127.evil.com are rejected. test/host-guard.test.ts asserts exactly this — isLoopbackHost('127.0.0.1.nip.io') === false, and the guard returns 403 FORBIDDEN_HOST for it. (The fix lives in config.ts; this line is just the call site.)
| if (trimmed.startsWith('[')) { | ||
| const end = trimmed.indexOf(']'); | ||
| return (end > 0 ? trimmed.slice(1, end) : trimmed.slice(1)).toLowerCase(); |
There was a problem hiding this comment.
hostnameFromAuthority accepts any string that starts with [ and returns the text up to the first ] without validating the remaining authority syntax. A malformed Host such as [::1]evil.com is therefore normalized to ::1, and hostnameAllowed then treats it as loopback. Because this middleware catches URL parse failures and falls back to the Host header, malformed bracketed authorities can bypass the guard for clients/proxies that forward such headers instead of being rejected with FORBIDDEN_HOST.
Useful? React with 👍 / 👎.
| normalizedBindHost: string, | ||
| allowedHosts: ReadonlySet<string>, | ||
| ): boolean { | ||
| if (isLoopbackHost(hostname)) return true; |
There was a problem hiding this comment.
.localhost subdomains as loopback
The guard claims loopback names are always allowed, but it delegates to isLoopbackHost, which only recognizes localhost, 127.*, and ::1. Valid localhost-domain names like tenant.localhost (defined as localhost names under RFC 6761 and commonly used for local dev) are rejected with 403 FORBIDDEN_HOST in the default loopback configuration unless users manually add them to VIS_ALLOWED_HOSTS, which is a functional regression introduced by this host check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in d1832a7 — isLoopbackHost now treats *.localhost as loopback (normalized.endsWith('.localhost')) per RFC 6761, so tenant.localhost is allowed rather than 403'd. test/host-guard.test.ts covers it (allowed → 200).
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Re-lands the Host-header guard on top of the agent-core protocol rewrite, which reorganized the vis server and dropped the earlier version of this change. The vis server binds to loopback and serves no auth token by default, so without a Host check any page the user visits could rebind its hostname to 127.0.0.1 and read or delete local agent sessions cross-origin. Design: the guard is installed only when no auth token is configured. A non-loopback bind already requires VIS_AUTH_TOKEN (config.ts), and a token defeats rebinding on its own (the attacker's page cannot read it cross-origin), so a Host allow-list in token mode would only break legitimate LAN / wildcard access without adding security. In the no-token loopback case the guard allows loopback names, the configured bind host, and VIS_ALLOWED_HOSTS entries; everything else gets 403 FORBIDDEN_HOST. Hardening folded in from review of the prior iteration: - isLoopbackHost matched any host starting with `127.`, so `127.0.0.1.nip.io` / `127.evil.com` were treated as loopback and bypassed the guard. Now a strict 127.0.0.0/8 dotted-quad match. - hostnameFromAuthority split unbracketed authorities at the first colon, truncating bare IPv6 literals (`2001:db8::10` -> `2001`); and a malformed bracketed authority like `[::1]evil.com` normalized to the loopback `::1`. Both are fixed: bare IPv6 literals are returned whole, and trailing junk after `]` makes the authority fail every allow check. - `*.localhost` (RFC 6761) is now treated as loopback instead of 403'd. Adds test/host-guard.test.ts covering authority parsing, the loopback matcher (including the 127.-prefix and bracket bypasses), the guard's allow/deny behavior, and the no-token-vs-token wiring in createApp. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aaronjmars
force-pushed
the
security/vis-server-dns-rebinding-host-guard
branch
from
b741fce to
d1832a7
Compare
|
@codex review Rebased onto current Design change — guard only in no-token mode. Rather than maintaining a Host allow-list for non-loopback binds, the guard is now installed only when no Bypasses fixed (both were real):
Also addressed:
New coverage in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1832a7829
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (trimmed.length === 0) return null; | ||
| if (trimmed.startsWith('[')) { | ||
| const end = trimmed.indexOf(']'); | ||
| if (end < 0) return trimmed.toLowerCase(); // unterminated bracket → cannot match |
There was a problem hiding this comment.
Treating an unterminated bracket host as a pass-through string here creates a guard bypass: hostnameFromAuthority('[::1') returns '[::1', and isLoopbackHost later strips brackets, turning it into '::1' (loopback) so the request is accepted instead of rejected. This means malformed Host values can still satisfy the allow check in no-token mode, contrary to the comment that malformed authorities "cannot match".
Useful? React with 👍 / 👎.
| normalized === 'localhost' || | ||
| // RFC 6761: `localhost.` and any `*.localhost` name resolves to loopback. | ||
| normalized.endsWith('.localhost') || |
There was a problem hiding this comment.
With the new host guard enabled in no-token mode, requests using valid FQDN loopback spellings like localhost. are rejected with FORBIDDEN_HOST because loopback detection only accepts exact localhost or *.localhost and never normalizes a trailing DNS root dot. This is a functional regression from pre-guard behavior (which accepted any host) and can block legitimate local clients that preserve the trailing dot form.
Useful? React with 👍 / 👎.
1 participant
Summary
The vis debug server (
apps/vis/server) binds to loopback by default and, in thatmode, serves the
/api/*routes with no auth token (resolveVisAuthTokenreturnsundefinedfor loopback; the bearer-auth middleware is only mounted when a token isset). The app has no Host-header / origin validation, so it is reachable cross-origin
via DNS rebinding: a website the user visits can rebind its own hostname to
127.0.0.1and, after the browser's DNS cache flips, talk to the local vis server assame-origin. This PR adds a Host-header allow-list guard that rejects requests whose
Host is neither a loopback name, the configured bind host, nor an entry in a new
VIS_ALLOWED_HOSTSenv var.Impact
While
vis-serveris running (defaulthttp://127.0.0.1:3001,auth=disabled), amalicious page can, through DNS rebinding, reach every
/api/*route with nocredential:
GET /api/sessions,/api/sessions/:id,/api/sessions/:id/wire,/api/sessions/:id/tool-results/:toolCallIdexpose full session wire logs — systemand user prompts, file contents the agent read, and tool outputs (which can contain
tokens/secrets the agent handled).
DELETE /api/sessions/:idandDELETE /api/sessionslet the same page delete asingle session or clear all of them.
The server already recognizes that non-loopback binding is dangerous (it requires
VIS_AUTH_TOKENand throws otherwise), but loopback binding was treated as inherentlysafe. DNS rebinding defeats that assumption because the browser still sends the
attacker's original hostname in the
Hostheader.Location
apps/vis/server/src/app.ts—createApp()mounted routes with no Host validation.Auth state set in
apps/vis/server/src/config.ts(resolveVisAuthToken).Fix
apps/vis/server/src/host-guard.ts: a Hono middleware that checks the request'sHost header / URL authority against the configured bind host. Loopback names
(
localhost,127.0.0.0/8,::1) are always allowed; a non-loopback bind host isallowed; additional hosts can be allow-listed via
VIS_ALLOWED_HOSTS(comma list,for reverse-proxy/custom-DNS setups). Anything else gets
403 FORBIDDEN_HOST.app.tsmounts the guard withapp.use('*', …)before any route, so both the APIand the static SPA are covered.
index.tspasses the resolved bind host andVIS_ALLOWED_HOSTSthrough.No behavior change for legitimate local use: requests to
localhost/127.0.0.1(theway the bundled web UI talks to the server) keep working; the existing
"keeps loopback-only local API access open" test still passes.
Detected by
Aeon — manual review of the localhost HTTP-server attack surface (Host-header /
DNS-rebinding axis). Semgrep, TruffleHog, and OSV were also run; this finding came from
manual review, not a scanner rule.
deletion as impact.
Verification
Added regression tests to
apps/vis/server/test/app.test.ts(the existing"vis server access controls" suite) covering foreign-Host rejection (GET + DELETE),
loopback allow, and
VIS_ALLOWED_HOSTS.pnpm --filter @moonshot-ai/vis-server test→test/app.test.ts9/9 pass(5 existing access-control tests + 4 new), so the existing
"keeps loopback-only local API access open" behavior is preserved.
@hono/node-serverover a raw socket:Host: localhost/127.0.0.1→200;Host: attacker.example(the rebinding case,GET + DELETE) →
403. node-server builds the request URL from the client's Hostheader, so this is the production path.
tsc --noEmitclean under this repo's tsconfig options (strict,verbatimModuleSyntax,noUncheckedIndexedAccess, bundler resolution).(Note: the rest of the
vis-serversuite has 4 unrelatedwire-loader.test.tsfailures in my sandbox — I'm on Node 22 and the repo requires ≥24.15, so my locally
built
@moonshot-ai/agent-coredrifts the wire record-name migration. Those reproducewith this PR's changes reverted and don't touch any file in this PR.)
Happy to adjust the approach (e.g. enforce only in loopback mode, or rename the env
var) if you'd prefer a different shape.
Filed by Aeon.